feat(dashboard): remove tooltip from root key permission toggle#2618
Conversation
unrenamed
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
@unrenamed is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1)
335-342: Fix indentation for consistent code formatting.
The indentation of these lines appears to be inconsistent with the rest of the file. Consider adjusting the indentation to match the surrounding code style.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1 hunks)
🔇 Additional comments (1)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1)
335-342: LGTM! Changes successfully address the tooltip overflow issue.
The implementation successfully:
- Removes the tooltip functionality that was causing overflow issues
- Maintains the core permission toggle functionality
- Preserves responsive layout and styling
- Keeps the UI clean and functional
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1)
335-342: Consider enhancing screen reader support.
The removal of tooltips in favor of always-visible descriptions is a good improvement. Consider adding aria-description to enhance screen reader support:
<div className="w-1/3 flex items-center gap-2">
<Checkbox
checked={checked}
onCheckedChange={() => {
setChecked(!checked);
}}
+ aria-description={description}
/>
<Label className="text-xs text-content">{label}</Label>
</div>📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1 hunks)
🔇 Additional comments (2)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (2)
335-342: LGTM! Improved accessibility and responsive layout.
The simplified structure maintains good accessibility with proper label association and clear visual hierarchy. The responsive layout ensures readability across different screen sizes.
335-342: Confirms resolution of tooltip overflow issue.
The changes successfully address the tooltip overflow issue (#2234) by:
- Removing the problematic tooltip
- Implementing an always-visible description with proper responsive layout
- Maintaining clear information hierarchy without compromising UX
This is a solid improvement to the permission toggle UI.
|
Could you remove them here too? |
|
I might not be around tomorrow to review, so I'll award the points now, trusting you to fix it /award 150 |
|
Awarding unrenamed: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/unrenamed |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/permission-manager-card.tsx (1)
107-109: Consider adding a comment about the width ratio decision.The 1/3 and 2/3 width split provides a good balance for the content, but it would be helpful to add a brief comment explaining this design decision for future maintainers.
- <div className="w-1/3 flex items-center gap-2"> + {/* Action takes 1/3 width, description takes 2/3 for optimal readability */} + <div className="w-1/3 flex items-center gap-2">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/permission-manager-card.tsx(1 hunks)
🔇 Additional comments (2)
apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/permission-manager-card.tsx (2)
104-109: LGTM! Clean implementation of the permission display.
The new layout with the Check icon and direct Label rendering provides a cleaner, more consistent interface. This aligns well with the PR objective of removing tooltips and matches the requested changes.
Line range hint 15-16: Future improvements noted in TODOs.
The component has planned enhancements for accordion functionality and empty state visualization. While outside the scope of this PR, these would further improve the user experience.
Let's check if there are any existing issues tracking these improvements:
Would you like me to create GitHub issues to track these planned improvements?
Also applies to: 29-29
✅ Verification successful
Consider tracking planned UX improvements
The code contains two TODO comments for future enhancements:
- Adding accordion functionality with an
expandableprop - Adding visualization for empty permission states
Since there are no existing issues or documentation discussing these features, consider creating dedicated issues to track these improvements for better user experience.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related issues about accordion or empty state
gh issue list --search "in:title accordion permission" --limit 5
gh issue list --search "in:title empty permission state" --limit 5
Length of output: 135
Script:
#!/bin/bash
# Let's first check the actual TODO comments in the file
rg -A 2 "TODO" "apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/permission-manager-card.tsx"
# Also search for any related existing issues or discussions in the codebase
rg -i "accordion.*permission|permission.*accordion|empty.*state.*permission" -g "*.{md,mdx}"
Length of output: 539
|
Awarding unrenamed: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/unrenamed |
2 participants
What does this PR do?
Fixes #2234
Type of change
How should this be tested?
/settings/root-keys/new;Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
permissionNameprop.Bug Fixes